feat(validator): commit pending MIG-mode change via targeted GPU reset - #2603
feat(validator): commit pending MIG-mode change via targeted GPU reset#2603lexfrei wants to merge 1 commit into
Conversation
|
Just linking back to the ongoing discussion at #2600 (comment) (thanks for the additional context there!). Let's revisit this PR once we decide on a direction for #2600. |
7a40a92 to
95e02db
Compare
95e02db to
c08f31a
Compare
📝 WalkthroughWalkthroughAdds an opt-in Merge Risk: 🟡 Moderate · up to The opt-in MIG commit path can report validation success after GPU discovery, MIG query, or reset failures, and it can ignore the configured driver root, preventing the targeted reset when the container path differs. That can leave pending MIG mode uncommitted and block creation of MIG-backed vGPU devices, so the PR is not merge-ready until these correctness issues are fixed or explicitly accepted. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Team
Run ID: 7e1a4c59-2ac1-48c9-94d9-1a9c772c0d32
📒 Files selected for processing (2)
cmd/nvidia-validator/main.gocmd/nvidia-validator/main_test.go
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review.
| // running compute work or fail to commit a pending MIG-mode enable. A reset is | ||
| // warranted only for an uncommitted MIG-mode enable on a GPU with no VFs and | ||
| // no running compute processes. | ||
| func TestShouldResetForMIGCommit(t *testing.T) { |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Add a commitMIGMode regression test.
TestShouldResetForMIGCommit tests only the predicate. It does not test GPU enumeration, process-query failures, reset command selection, or aggregated reset errors in commitMIGMode.
Add an injected PCI and command boundary. Verify that eligible GPUs reset once, guarded GPUs never reset, and reset failures reach the caller. This prevents a regression that resets an ineligible GPU or skips an eligible reset.
As per path instructions: “Flag … new operand or CRD behaviour that ships with no regression test.”
Source: Path instructions
| if err := commitMIGMode(hostDriver); err != nil { | ||
| log.Warnf("Unable to commit pending MIG mode, continuing: %v", err) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Return the MIG commit error.
If GPU discovery, MIG-mode query, or GPU reset fails, this path logs the error and continues as success. This suppresses retry of the pending MIG transition during validation and can leave the GPU uncommitted.
As per path instructions: “Flag an error that is logged and then returned as success. That suppresses the retry.”
Source: Path instructions
| // and the WSL path). | ||
| func vgpuNvidiaSMI(hostDriver bool, hostRootCtrPath string) (driverRoot, nvidiaSMI string, err error) { | ||
| if !hostDriver { | ||
| return defaultDriverInstallDir, "nvidia-smi", nil |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use the configured container driver root.
When DRIVER_INSTALL_DIR_CTR_PATH differs from /run/nvidia/driver, vgpuNvidiaSMI returns the wrong chroot root. The MIG query then fails, validation logs the error, and the requested GPU reset does not run.
cmd/nvidia-validator/main.go#L1884-L1884: returndriverInstallDirCtrPathFlagfor container drivers.cmd/nvidia-validator/main_test.go#L723-L727: set a non-defaultdriverInstallDirCtrPathFlagand assert thatvgpuNvidiaSMIreturns it.
As per path instructions: “Flag assertions that would still pass if the behavior under test were broken.”
📍 Affects 2 files
cmd/nvidia-validator/main.go#L1884-L1884(this comment)cmd/nvidia-validator/main_test.go#L723-L727
Source: Path instructions
On NVIDIA Ampere GPUs (A100, A30), enabling MIG mode requires a GPU reset and the driver attempts that reset itself. The attempt is refused while any application is using the GPU, which leaves the card reporting mig.mode.pending=Enabled with mig.mode.current not Enabled: the change is requested but never applied, and MIG-backed vGPU devices cannot be created until some later reset commits it. That is the window this addresses. Add an opt-in step to vGPU Manager validation that commits such a pending-but-uncommitted MIG-mode enable via a targeted 'nvidia-smi --gpu-reset'. It runs before validation waits for the vGPU stack to come up; the reset is skipped while VFs are present. The reset is destructive, so it is tightly gated: it runs only when explicitly enabled (commit-mig-mode-with-gpu-reset, off by default), only on the vGPU path, and only on a GPU whose MIG-mode enable is requested but not yet applied, that has no SR-IOV VFs enabled, and that has no running compute processes. It is best-effort: reset failures are logged and validation continues, preserving current behavior for setups that commit MIG mode by other means. Scope: this commits a mode change that was already requested. It does not recreate MIG devices, which do not survive a reboot and are recreated by whatever manages the MIG layout, not by a reset. MIG mode itself is persistent across reboots on Ampere via an InfoROM status bit, and from Hopper onward enabling MIG mode no longer requires a GPU reset. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la>
c08f31a to
f28c518
Compare
2 participants
Description
On Ampere (A100, A30) enabling MIG mode needs a GPU reset, and the driver tries that reset itself. It doesn't always work: the reset is refused while the GPU is in use, and under passthrough the hypervisor can disallow it. The GPU then sits with
mig.mode.pending=Enabledandmig.mode.currentnotEnabled, so MIG-backed vGPU devices can't be created until something resets it later. Right now that means a hand-rolled host-side step.This adds an opt-in step to vGPU Manager validation that commits such a pending enable with a targeted
nvidia-smi --gpu-reset. It runs before the wait for SR-IOV VFs, and it skips any GPU that already has VFs.What this is not: it does not recreate MIG state lost to a reboot, and it isn't needed for that. MIG mode survives a reboot on Ampere via an InfoROM status bit, and from Hopper on, enabling it no longer needs a reset. MIG instances don't survive on any generation, but recreating those is
mig-parted's job.Destructive GPU reset, why it is safe here
nvidia-smi --gpu-resetclears GPU hardware and software state; the manual describes it as a way to avoid a machine reboot. It does not tear down running work: nvidia-smi refuses the reset while any application is using the device, and the MIG guide says to kill those processes and retry. The step is off by default and gated because a validator should not do a privileged device-level operation implicitly, and because clearing device state breaks anything holding the GPU. It resets a GPU only when all of these hold:--commit-mig-mode-with-gpu-reset(envCOMMIT_MIG_MODE_WITH_GPU_RESET), defaultfalse. Unset, behavior is identical to today: no reset,validate()unchanged.commitMIGModeis reached only fromVGPUManager.validate(), which early-returns for non-vm-vgpuworkloads.mig.mode.pending == Enabledandmig.mode.current != Enabled. Something else already requested the change; this only commits it, never decides it.--query-compute-apps, so it only sees compute clients.nvidia-smi --gpu-resetrefuses to reset a GPU while any application is using it, including the graphics and monitoring clients this query misses. The manual also says an individual reset may not be feasible while compute apps run on other GPUs; that comes back as a logged failure.Enabling it
The flag reads its env var from the
vgpu-managervalidation init container, so the genericvalidator.envdoes not reach it: that one is applied to the main validator container. The key that works isvalidator.vgpuManager.env, which the chart templates and the operator forwards oncase "vgpu-manager":That path works but is not discoverable.
validator.vgpuManageris absent fromvalues.yaml, which declares onlyenv,argsandplugin.envundervalidator, so the key never shows up inhelm show valuesand an operator can only find it by reading the chart template.validator.vfioPCIandvalidator.vgpuDeviceshave the same gap. Happy to send a separate PR declaring all three if you want them discoverable.In the steady state, MIG already committed or VFs present, it does nothing. Best-effort: reset failures are logged and validation continues, so setups that commit MIG mode out-of-band keep working as before.
Two open questions I'd rather raise than paper over, both discussed in #2600:
nvidia-smi -r, and NVIDIA's own MIG recipes enable VFs before setting MIG mode. So the guard is observed behavior without a verified rationale.Checklist
make lint): the changed package is clean,GOOS=linux golangci-lint run ./cmd/nvidia-validator/reports 0 issues. Repo-widemake lintwas not run locally.make validate-generated-assets): this change adds no API/CRD changes, so no generated assets change; the full target was not run.make validate-modules)Testing
gofmt,go build,go vet, andgolangci-lint runon the changed package, cross-compiledGOOS=linux(the validator is a Linux-only binary): clean, 0 issues. No asset orgo.mod/go.sumchanges.go test ./cmd/nvidia-validator/):TestNormalizePCIAddress(nvidia-smi 8-digit vs go-nvlib 4-digit PCI domains normalize to the same key),TestParseMIGModes(CSV parsing,[N/A], malformed rows, the pending-vs-current guard),TestMIGModeNeedsCommit(the enable-only commit predicate across every current/pending combination, including case-insensitivity), andTestShouldResetForMIGCommit(the reset-decision guard matrix over uncommitted-enable, VFs-present and workload-running).commitMIGModeitself has no test, including its off-by-default gate. That is the gap I'd fix first if this design survives the discussion in [Feature]: Persist MIG mode and SR-IOV vGPU VFs across reboot on the sandbox/vGPU path (operator-driven GPU reset) #2600.nvidia-smi --gpu-resetpath relies on CI and cluster testing. This is why the PR is opened as a draft/RFC.Refs #2600.